[TySan] Added a 'print_stacktrace' flag for more detailed errors #121756
Conversation
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: None (gbMattN) ChangesRaised in issue #121697 Full diff: https://github.com/llvm/llvm-project/pull/121756.diff 2 Files Affected:
diff --git a/compiler-rt/lib/tysan/tysan.cpp b/compiler-rt/lib/tysan/tysan.cpp
index 39d78e7c95e0cd..2786d826afb5f4 100644
--- a/compiler-rt/lib/tysan/tysan.cpp
+++ b/compiler-rt/lib/tysan/tysan.cpp
@@ -198,7 +198,7 @@ static void reportError(void *Addr, int Size, tysan_type_descriptor *TD,
if (pc) {
- bool request_fast = StackTrace::WillUseFastUnwind(true);
+ bool request_fast = StackTrace::WillUseFastUnwind(true) && !flags().print_stacktrace;
BufferedStackTrace ST;
ST.Unwind(kStackTraceMax, pc, bp, 0, 0, 0, request_fast);
ST.Print();
diff --git a/compiler-rt/lib/tysan/tysan_flags.inc b/compiler-rt/lib/tysan/tysan_flags.inc
index 98b6591f844ef0..be65c8e828794a 100644
--- a/compiler-rt/lib/tysan/tysan_flags.inc
+++ b/compiler-rt/lib/tysan/tysan_flags.inc
@@ -15,3 +15,6 @@
// TYSAN_FLAG(Type, Name, DefaultValue, Description)
// See COMMON_FLAG in sanitizer_flags.inc for more details.
+
+TYSAN_FLAG(bool, print_stacktrace, false,
+ "Include full stacktrace into an error report")
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
gbMattN
force-pushed
the
users/nagym/tysan-add-full-stack-trace-option
branch
from
f55dd6c to
6580b3f
Compare
goussepi
left a comment
goussepi
left a comment
There was a problem hiding this comment.
Is the issue maybe that stack trace are not being printed unless fast unwind is disabled ?
compiler-rt/lib/tysan/tysan.cpp
Outdated
| if (pc) { | ||
|
|
||
| bool request_fast = StackTrace::WillUseFastUnwind(true); | ||
| bool request_fast = |
There was a problem hiding this comment.
I thought "fastunwind" was unrelated to whether we print a stack trace or not ? I thought it controls the method of obtaining the stack trace.
There was a problem hiding this comment.
For some reason, setting it to false also gave a full stack printout
Thanks for pointing this out though, I've found the proper way of doing it!
goussepi
left a comment
goussepi
left a comment
There was a problem hiding this comment.
Great find! I guess now stack traces are being printed by default but before only one line was being printed ? The question is which behavior is best by default, should we still have the "print_stacktrace" option in case ?
I think it should match the existing behavior in other places i.e. full stacktrace off by default. |
|
That sounds goo to me! We now have the existing behaviour if you don't add |
For correctness that should be |
goussepi
left a comment
goussepi
left a comment
There was a problem hiding this comment.
Does Tysan support passing options through environment variables, such as TYSAN_OPTIONS? If so, it might be helpful to add a test to check if print_stacktrace works as intended. This could be done using a lit substitution variable like %env_tysan_opts (if not already present?).
|
My previous comment was a mistake, |
There was a problem hiding this comment.
I think eventually we will need to introduce a %env_tysan_opts lit substitutions to protect from environment variables affecting test results, but can be done in a separate patch.
There was a problem hiding this comment.
Ah! We actually have that already it seems. I'll change that
compiler-rt/lib/tysan/tysan.cpp
Outdated
There was a problem hiding this comment.
nit: While you are here, maybe drop the newline at the top here?
gbMattN
force-pushed
the
users/nagym/tysan-add-full-stack-trace-option
branch
from
b3d03da to
af53f83
Compare
gbMattN
force-pushed
the
users/nagym/tysan-add-full-stack-trace-option
branch
from
cacfa49 to
d8c7734
Compare
5 participants
Raised in issue #121697